[tool] Change gradle-check logic to enforce alignment of java versions and a minimum (17) #10206
Conversation
…va version alignment
reidbaker
requested review from
ash2moon,
bparrishMines,
camsim99,
gmackall,
mboetger and
tarrinneal
as code owners
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the Java compatibility version to 17 across multiple Android packages and enhances the gradle-check tool to enforce this new minimum version and ensure version alignment within build.gradle files. The changes also include numerous formatting updates in Dart files, likely from an auto-formatter.
My review focuses on the new validation logic in gradle-check. While the new checks are a great improvement, I've identified a potential issue in how Java versions are parsed, which could lead to incorrect validation in some edge cases. I've provided a suggestion to make the parsing more robust.
reidbaker
removed request for
ash2moon,
bparrishMines,
camsim99,
gmackall,
mboetger and
tarrinneal
gmackall
changed the title
| // Some java versions have the format VERSION_1_8 but we dont need to handle those | ||
| // because they are below the minimum. | ||
| final RegExp javaVersionMatcher = | ||
| RegExp(r'JavaVersion.VERSION_(?<javaVersion>\d+)'); |
There was a problem hiding this comment.
Rather than tagging this on at the end, you could restructure the earlier somewhat to avoid having multiple passes over the same file looking for the same lines different ways, which I feel like makes the code harder to follow. You could start this whole method with something like (this is freehand so is probably wrong, but should give the idea):
final Map<String, String> javaVersionValues = <String, String>{};
final RegExp javaVersionMatcher =
RegExp(r'(?<value>\w+)\s*=\s*JavaVersion.VERSION_(?<javaVersion>\d+)');
for (final String line in gradleLines) {
final RegExpMatch? match = javaVersionMatcher.firstMatch(line);
if (!_isCommented(line) && match != null) {
if (foundVersion != null) {
javaVersions[match.namedGroup('value')!] = match.namedGroup('javaVersion')!;
}
}
}Then all of the gradleLines.any((String line) => line.contains(...) checks above are just key checks in that map, and you can use the same map below for the version checks.
There was a problem hiding this comment.
Organizationally what if I pulled each check into its own method then had
_validateCompatibilityVersions(List<String> gradleLines) {
_validateTargetAndSourceFormat();
_validateOptionalKotlinOptionsFormat();
_validateHomogenousAndMinimumJava()
}
That might help with readability and make it clear when formatting cares about the values vs the structure?
There was a problem hiding this comment.
If you prefer the current code structure, breaking out helpers so it's not one big method SGTM.
| '$indentation${minimumJavaVersionError.split('\n').join('\n$indentation')}'); | ||
| return false; | ||
| } | ||
| if (!javaVersions.every((String element) => element == '$version')) { |
There was a problem hiding this comment.
I would strongly prefer making this the first check instead of the second, since if you don't already know there's going to be a check that they all match, the check above that only the first element is the right version seems wrong.
There was a problem hiding this comment.
Yeah I can make this change :) standby
2 participants
Hold review/merge until #10201 has merged.
This pr is code review feedback from #10201 (comment)
Pre-Review Checklist
[shared_preferences]pubspec.yamlwith an appropriate new version according to the [pub versioning philosophy], or I have commented below to indicate which [version change exemption] this PR falls under[^1].CHANGELOG.mdto add a description of the change, [following repository CHANGELOG style], or I have commented below to indicate which [CHANGELOG exemption] this PR falls under[^1].///).